Support MIN and MAX in the datafusion planner#66
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| use lance_graph::{CypherQuery, ExecutionStrategy}; | ||
| use std::collections::HashMap; | ||
| use std::sync::Arc; | ||
|
|
There was a problem hiding this comment.
Could you keep this blank line?
| .unwrap() | ||
| } | ||
|
|
||
| fn add_new_person_to_dataset( |
There was a problem hiding this comment.
After reviewing the tests you added for MIN and MAX, I don't think this method is necessary: it is just used to add a new person to the dataset, but the current person dataset has already had 5 rows (so good enough to find max/min age). Could you remove this helper function and update the tests accordingly?
There was a problem hiding this comment.
Initially, I thought we should add an extra item that shares the same city value as one of the existing five items. However, after checking the documentation, I realized that RecordBatch is immutable. Adding a new value would therefore require creating a new RecordBatch, which is inefficient. I agree that we should remove this method. Updated.
| @@ -1,10 +1,10 @@ | |||
| use arrow::compute::kernels::numeric::add; | |||
There was a problem hiding this comment.
Looks like this is unused in this test file: could you remove it?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_max_without_alias_has_descriptive_name() { |
There was a problem hiding this comment.
Generally, MIN and MAX are sharing very similar features, so I think we don't need this many new tests. How about just keeping
- test_min_property
- test_max_property
- test_min_max_with_grouping (testing min and max with grouping in one method)
We can safely remove other MIN/MAX tests.
|
@leiyuou Thanks for the work! Just left some minor comments (mainly about integration test organization). |
ChunxuTang
left a comment
ChunxuTang
left a comment
There was a problem hiding this comment.
lgtm. Thanks for the work!
3 participants
Support MIN and MAX in the datafusion planner
Add unit test and integration test
Add a helper function to add one more people in dataset.